Skip to content

Feature/pre 3440 multi shop configuration - #331

Open
adumont-payplug wants to merge 9 commits into
developfrom
feature/PRE-3440_multi_shop_configuration
Open

adumont-payplug wants to merge 9 commits into
developfrom
feature/PRE-3440_multi_shop_configuration

Conversation

@adumont-payplug

@adumont-payplug adumont-payplug commented Sep 17, 2026

Copy link
Copy Markdown
Collaborator

Description

Multi-shop support: a Sylius installation can now hold several PayPlug gateway configurations of the same type, each connected to its own PayPlug account and scoped to its own set of channels.

Until now the plugin assumed one PayPlug account per installation. AbstractGatewayConfigurationType refused the creation of a second gateway config for a factory name that already existed, and everything downstream — the API client, the /account payload, the UPC configuration repository — resolved credentials by factory name alone. That is fine with one account, and silently wrong with several: a name-based lookup returns an arbitrary config, so a request for channel A can be signed with channel B's credentials.

This PR replaces the installation-wide uniqueness rule with a per-channel one, then threads the payment method (rather than the factory name) through every place that needs to know which account it is talking to, and gives the admin the two things that become necessary once several accounts coexist: seeing which account a gateway is connected to, and disconnecting one of them without touching the others.

Motivation: merchants running several shops on one Sylius installation need one PayPlug account per channel.

Related issue(s): PRE-3440 — includes PRE-3628, PRE-3629, PRE-3631, PRE-3632, PRE-3682, PRE-3683, PRE-3685.


PRE-3628 — per-channel gateway uniqueness

The rule is now: a channel may be linked to at most one enabled gateway config per factory type. Two CB gateways may coexist and both be enabled as long as their channel sets are disjoint; different factory types never conflict.

  • New Checker/GatewayChannelConflictChecker — matches on channel code rather than object identity, ignores disabled gateways on both sides, and handles the not-yet-persisted subject (no id ⇒ can never match a rival).
  • New Gateway/Form/Extension/PaymentMethodTypeExtension — carries both the conflict rule and the base-currency rule on the root payment-method form. That move is the crux: Sylius adds channels from CoreBundle's own type extension, i.e. after gatewayConfig, so a listener inside gatewayConfig.config runs before enabled and channels are submitted and can only ever see persisted data. Root POST_SUBMIT is the first point where the submitted channel set, the submitted enabled flag and the mapped gateway config all exist.
  • AbstractGatewayConfigurationType loses its PRE_SUBMIT listener, the canBeCreated() / checkCreationRequirements() pair and two constructor dependencies; the per-gateway currency policy stays where it belongs (one hook per gateway type) and is read back by the extension.
  • The CB base-currency gate now reads the mapped config through PayPlugGatewayFactory::resolveDisplayMode() instead of the unmapped DISPLAY_MODE_FIELD form key, which never reaches the persisted config.
  • New PaymentMethodRepository::findEnabledByGatewayName().
  • Translation form.only_one_gateway_allowedform.gateway_channel_conflict (en/fr/it).

PRE-3629 — claimed channels are unselectable in the picker

POST_SET_DATA on the root form replaces the channels child with a copy carrying a choice_attr closure, so a channel already held by another enabled gateway of the same factory renders disabled with a title naming the claiming payment method.

Channels the edited payment method already holds are deliberately left selectable: browsers do not submit disabled checkboxes, so disabling a checked one would silently drop that channel on save. Pre-existing overlaps are reported by the submit-time rule instead. Both answers come from the same claims() lookup, which is what keeps the picker and the validator in step.

PRE-3682 / PRE-3683 / PRE-3685 — scoping credentials to the payment method

  • PayPlugApiClientFactoryInterface::create(string $factoryName) is removed from the interface. createForPaymentMethod() is now the only way application code can obtain a client — the compiler, not review, is the guard against reintroducing a channel-ambiguous lookup. The concrete create() survives as @internal purely for the client.xml service-factory definitions (the remaining open half of PRE-3682, tracked separately).
  • SupportedMethodsProvider fetches /account per gateway config, memoized by persisted id (falling back to spl_object_id for unflushed configs) instead of once per call — previously the first method's account governed every later one in the list. The payment_methods sub-key is resolved from the config the payload was fetched for.
  • New Upc/ScopedConfigurationRepositoryInterface — UPC's IConfigurationRepository takes no context on any method (it was written for one account per installation). Rather than widen a shared contract that other plugins consume, the scope is carried Sylius-side by a sub-interface with withGatewayConfig() / forPaymentMethod() withers: the repository is a shared service, and a mutable scope would leak across requests — IPN and background token refresh being exactly where that would go unnoticed.
  • UnifiedApiPaymentCreatorInterface::createPayment(), OperationStatusFetcherInterface::getOperation(), the UHF command handlers, HostedFieldsWebhookNotificationHandler, IpnAction, OneClickAction, IntegratedPaymentController, PaymentStateResolver, CaptureAuthorizedPaymentProcessor and the Oney/permission validators all take or resolve the payment method now.

PRE-3631 — the connected account, per gateway

New Auth/IdTokenEmailExtractor reads the email claim out of the OAuth id_token at callback time and UnifiedAuthenticationController writes it to the gateway config as account_email; a read-only connected_account.html.twig renders it on the update screen of all seven gateways.

Worth knowing: /account carries no email (verified live — the payload is id, company_ref, country, object, is_live, configuration, permissions, payment_methods), and neither does the client-credentials token used for background calls. The interactive authorization-code exchange is the only place the address exists, which is why it is captured at login rather than fetched on demand — same approach as the PrestaShop module. The extractor is total (any malformed input returns null) and deliberately does not verify the signature: the token arrives as the direct response body of a server-to-server POST, never via the browser, and the claim is display text, not an authorization decision. A gateway connected before this change shows the "re-authenticate" placeholder until the merchant reconnects.

Requires payplug/unified-plugin-core ^1.1.2, where TokenOutput gained a nullable idToken — earlier versions drop id_token from the token response entirely. Constraint bumped accordingly.

PRE-3632 — disconnect one gateway

New UnifiedLogoutController + Auth/GatewayConnectionRevoker — the inverse of the OAuth callback, scoped to a single gateway config. It clears live_client, test_client and account_email, drops both cached UPC tokens, and disables the payment method.

  • hfIdentifier is cleared only when the config is a CB gateway with Hosted Fields selected; elsewhere it is a merchant-typed value, not account-bound state. live, oneClick, deferredCapture, the display-mode flags and fees_for are untouched.
  • Distinct from the renew_oauth checkbox, which immediately mints new credentials — logout ends with none.
  • Two consequences worth flagging for QA: since PRE-3629 only counts enabled gateways, logging out releases that gateway's channels for another config to claim; and because PaymentMethodValidator::process() only ever disables, the merchant must re-tick "Enabled" by hand after reconnecting.
  • GET, not POST: the button is rendered inside the Sylius payment-method <form>, where a nested <form> would be invalid HTML. The CSRF token travels in the query string, the same shape as Sylius's own sylius_admin_shipment_resend_confirmation_email. security.csrf.token_manager is injected with @? — it is absent when CSRF protection is off, and a hard reference would break container compilation for such an app.

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 💥 Breaking change (fix or feature that causes existing functionality to change and that could impact other libs)
  • 📦 Dependency update

Breaking changes for anyone extending the plugin

Removed / changed Replacement
PayPlugApiClientFactoryInterface::create(string $factoryName) createForPaymentMethod(PaymentMethodInterface $pm)
UnifiedApiPaymentCreatorInterface::createPayment($dto) createPayment($dto, PaymentMethodInterface $method)
OperationStatusFetcherInterface::getOperation($id) getOperation($id, PaymentMethodInterface $method)
AbstractGatewayConfigurationType::__construct()$gatewayConfigRepository and $requestStack dropped translator only
shouldValidateBaseCurrency() / baseCurrencyViolationMessage()protectedpublic, now take the mapped config same hooks, new visibility/shape
$gatewayFactoryName property on the 8 configuration types no longer read; the factory name comes off the gateway config
Translation key form.only_one_gateway_allowed form.gateway_channel_conflict (%channel%, %payment_method%)
Injecting PayplugUnifiedCore\Contracts\IConfigurationRepository ScopedConfigurationRepositoryInterface, scoped per payment method

Checklist

Code Quality

  • Code is linted and formatted
  • No unnecessary commented-out code or debug logs
  • No hardcoded values (use env variables or config)

Testing

  • Unit tests added / updated

New suites: GatewayChannelConflictCheckerTest, PaymentMethodTypeExtensionTest, IdTokenEmailExtractorTest, GatewayConnectionRevokerTest, UnifiedLogoutControllerTest, IntegratedPaymentControllerTest, plus scoping coverage added to the UPC, API-client-factory and SupportedMethodsProvider tests.

Security & Ops

  • No sensitive data or secrets introduced
  • Logging and error handling are appropriate

Manual test plan

  1. Create two CB payment methods on disjoint channels, authenticate each against a different PayPlug account → both save and both stay enabled.
  2. Try to give the second one a channel the first already holds → the checkbox is rendered disabled, and submitting the overlap anyway is refused with the conflict error on the channels field.
  3. Disable the first → its channels become selectable for the second.
  4. Check out on each channel → the payment is created on that channel's account (SupportedMethodsProvider amount limits and allowed countries follow the right account too).
  5. On each update screen, the connected account email is displayed; "Disconnect this account" clears that gateway's credentials and disables it, leaving the other gateway connected and working.

@adumont-payplug adumont-payplug left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review pass on the full diff

Reviewed in five passes: form/validation (PRE-3628/3629), credential scoping (PRE-3682/3683/3685), auth controllers + extractor + revoker (PRE-3631/3632), tests, config/translations. Full PHPUnit suite run under PHP 8.2: 589 tests, green.

What holds up

Several of the load-bearing claims in the description were checked against vendor source rather than taken on trust, and all of them hold:

  • The wither immutability is watertight. SyliusUpcConfigurationRepository uses clone $this and assigns on the clone; nothing mutates $this. Every consumer in src/ scopes before use — there is no unscoped configurationRepository-> call left. The cross-tenant leakage failure mode during IPN is closed.
  • The token-cache purge is correct, which is easy to get wrong: TOKEN_CACHE_KEY_PREFIX matches UPC's TokenManager exactly, and both the revoker and TokenManager go through the same shared SyliusTokenCache, so PSR-6 key sanitization is symmetric on write and delete. GatewayConnectionRevokerTest exercising a real cache over an ArrayAdapter instead of mocking ITokenCache is the right call.
  • The @? CSRF argument is sound. Sylius' own CsrfProtectionEnabledExtension::isCsrfProtectionEnabled() is literally $this->container->has('security.csrf.token_manager') — the template gate and the controller null check are the same condition, so there is no window where the link omits a token the controller then demands. The route also sits behind /admin, so the token is defence-in-depth, not the only authorization.
  • IdTokenEmailExtractor is genuinely total. Every branch checked: json_decode without JSON_THROW_ON_ERROR returns null for non-UTF8 and for depth > 512; a segment length ≡ 1 mod 4 produces a pad base64_decode(..., true) rejects; filter_var never throws.
  • Errors added to channels do not bubble to the root (ChoiceType sets error_bubbling => false on the type itself), and Form::add() inside POST_SET_DATA does re-map data into the replaced child (lockSetData is only on during PRE_SET_DATA). Both docblock claims are accurate.
  • GatewayChannelConflictChecker is the strongest piece here. The asymmetry between findConflicts() (bails on a disabled subject) and findClaimedChannels() (unconditional, minus the subject's own channels) is non-obvious and correct.
  • SupportedMethodsProvider is a real bug fix on its own — the old ??= let the first method's /account payload govern every later one in the list.

Two findings with no file in the diff

Three factory-name credential lookups the description does not list as remaining gaps. The breaking-change section says the only open half of PRE-3682 is client.xml's singletons. It isn't:

  • src/Provider/OneySupportedPaymentChoiceProvider.php:42findOneByGatewayName(OneyGatewayFactory::FACTORY_NAME)
  • src/Provider/Payment/ApplePayPaymentProvider.php:52 and :209 — same, for Apple Pay
  • src/Twig/OneyExtension.php:35findOneBy(['factoryName' => OneyGatewayFactory::FACTORY_NAME])

findOneByGatewayName() is setMaxResults(1)->getSingleResult(), so with two Oney or two Apple Pay gateways it returns an arbitrary one — the exact bug this PR exists to kill, in shop-facing code (Apple Pay merchant-session/domain validation, Oney simulation display). Not necessarily in scope to fix here, but they should be listed and ticketed. Separately: that method is typed ?PaymentMethodInterface but getSingleResult() throws NoResultException rather than returning null — pre-existing.

No CHANGELOG.md / UPGRADE.md entry. There is an eight-row breaking-change table for anyone extending the plugin — dropped constructor arguments, changed interface signatures, a removed translation key, protectedpublic hooks. CHANGELOG.md has a live ## [2.0.0] - Unreleased section and neither file was touched. Integrators will not read a PR body.


Assessment

Ready to merge: with fixes. The credential-scoping architecture is sound and the immutability, cache-key and form-mechanics claims all check out. One critical issue (see the IntegratedPaymentController thread) turns a previously dormant assumption into an exploitable one, and the form layer that enforces the new per-channel rule traded its only real-form test for an all-mock one.

On the plan itself: the four pre-justified tradeoffs — GET + query-string CSRF, unsigned id_token parsing, leaving already-held channels selectable, the wither-based scoped repository — all survive scrutiny, and in three cases the reasoning is more careful than the summary lets on. Where the plan under-reaches is that it treats "thread the PaymentMethodInterface through" as sufficient without asking where that PaymentMethodInterface comes from. IntegratedPaymentController is where that omission bites.

Comment thread src/Controller/IntegratedPaymentController.php
Comment thread src/Gateway/Form/Extension/PaymentMethodTypeExtension.php
Comment thread src/Checker/GatewayChannelConflictChecker.php
Comment thread src/Gateway/Form/Extension/PaymentMethodTypeExtension.php Outdated
Comment thread src/Gateway/Form/Extension/PaymentMethodTypeExtension.php Outdated
Comment thread translations/messages.en.yml Outdated
Comment thread config/services.yaml Outdated
Comment thread src/Auth/GatewayConnectionRevoker.php Outdated
Comment thread src/Auth/IdTokenEmailExtractor.php

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Code Review

Claude Code Review is paused for this repository. To reconnect it, an admin of this repository's GitHub organization (or the account owner, for personal repositories) who can also manage your Claude organization's Code Review settings needs to re-link GitHub in Code Review settings. This is a one-time step.

Tip: disable this comment in your organization's Code Review settings.

@adumont-payplug
adumont-payplug force-pushed the feature/PRE-3440_multi_shop_configuration branch from 5e10bf7 to 2aecfe0 Compare September 17, 2026 13:07
PRE-3628: scope gateway uniqueness validation per channel

PRE-3628: validate base currency against submitted channels

PRE-3628: fix CB base-currency gate to read mapped config

PRE-3628: address final review polish items

- de-dup CB base-currency form errors, not just flashes
- flash() no longer throws with no request/session
- drop 8 dead gatewayFactoryName property declarations
- suppress PHPMD unused-param on shouldValidateBaseCurrency()
- assert PaymentMethodTypeExtension::getExtendedTypes()
- tighten PaymentMethodRepository docblocks to list<>
@adumont-payplug
adumont-payplug force-pushed the feature/PRE-3440_multi_shop_configuration branch from 2aecfe0 to d9c957a Compare September 18, 2026 08:27

@jhoaraupp jhoaraupp left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the full multi-shop scoping chain (credentials/API client, gateway-conflict checker, OAuth connect/disconnect, and the Oney/Apple Pay/simulation providers) via 4 parallel deep-dives. Overall this is careful, well-documented work — the core resolution chain (PayPlugApiClientFactory, ScopedConfigurationRepositoryInterface, IntegratedPaymentController, IPN/webhook trust chain, GatewayChannelConflictChecker, GatewayConnectionRevoker, IdTokenEmailExtractor) is sound, fail-loud where it matters, and the new test suites explicitly assert cross-account/cross-channel isolation rather than just happy paths.

Two of the findings below are HIGH: real regressions of the exact bug class this PR sets out to close (an arbitrary account gets used instead of the channel-scoped one). They're in code paths adjacent to the ones this PR touched but outside this diff, which is presumably why they slipped through.

Summary: 2 HIGH, 4 MEDIUM, 6 LOW/NIT. Three of them sit in files this PR doesn't actually touch, so they can't be left as inline comments here — listed below instead.


[HIGH] RefundUnitsCommandCreatorDecorator::canOneyRefundBeMade() authenticates with an arbitrary Oney account, not the order's own (src/Creator/RefundUnitsCommandCreatorDecorator.php:46,106-111, not touched by this PR)

$this->oneyClient is the ambiguous singleton built via PayPlugApiClientFactory::create('payplug_oney')findOneBy(['factoryName' => ...]) — exactly the channel-ambiguous lookup this PR eliminates everywhere else. $lastPayment->getMethod() is already resolved a few lines above in fromRequest() and isn't threaded through here.

Failure scenario: merchant has Oney configured on channel A and channel B with two different PayPlug accounts (now legal since PRE-3628). Refunding an order from channel B within the 48h window calls retrieve() against whichever Oney config Doctrine's findOneBy returns first — if that's channel A's config, the refund is wrongly blocked (oney_transaction_less_than_forty_eight_hours) or errors, because it's authenticated against the wrong account.

-        private PayPlugApiClientInterface $oneyClient,
+        private PayPlugApiClientFactoryInterface $apiClientFactory,
...
-        $data = $this->oneyClient->retrieve($lastPayment->getDetails()['payment_id']);
+        $data = $this->apiClientFactory->createForPaymentMethod($lastPayment->getMethod())->retrieve($lastPayment->getDetails()['payment_id']);

[MEDIUM] CardController signs saved-card deletion with an arbitrary CB account, not the card's owning one (src/Controller/CardController.php:30-31,76, not touched by this PR)

Same ambiguous @payplug_sylius_payplug_plugin.api_client.payplug singleton, no channel/payment-method scoping. With two enabled CB configs on different channels/accounts, a card-deletion request gets signed with an arbitrary one of the two. PayPlug card tokens are account-scoped, so today's practical effect is a functional failure (delete silently fails, NotFoundException caught, "deleted_error" flashed) rather than cross-tenant leakage — but it's broken in exactly the multi-shop configuration this PR enables.

[LOW] PostSavePaymentMethodEventListener::onCreate() never runs PaymentMethodValidator::process(), so the new constraint's claimed backstop isn't reached on creation (src/EventListener/PostSavePaymentMethodEventListener.php:29-40, not touched by this PR)

HasNoGatewayChannelConflict's docblock (src/Gateway/Validator/Constraints/HasNoGatewayChannelConflict.php:13-16) says it's "the backstop for every other write path … where no form listener runs", but onCreate() only calls startOAuth() — a raw admin-API POST create bypasses both the admin form's POST_SUBMIT listener (no form involved) and this create path, so a conflicting config created that way isn't caught until the next admin update. Pre-existing gap (the old canBeCreated() check had the identical hole), not a regression — but worth either closing or softening the docblock's claim.


One more MEDIUM with no single anchor line: none of OneySupportedPaymentChoiceProvider, ApplePayPaymentProvider, CachedSimulationDataProvider, OneyExtension, IsOneyEnabledValidator, PayplugPermissionValidator have PHPUnit coverage (pre-existing gap, but this PR adds meaningfully new branching — channel resolution, ChannelNotFoundException handling — precisely in these files with no new/updated tests; exactly this kind of test would have caught the isOneyEnabled() finding below).

Positive notes worth calling out: PayPlugApiClientFactoryInterface dropping create(string $factoryName) entirely so the compiler blocks reintroducing the unsafe lookup is a great API-design call; ScopedConfigurationRepositoryInterface's withGatewayConfig()/forPaymentMethod() returning cloned instances (not mutating the shared singleton) correctly avoids state leaking across requests for IPN/background refresh; and the GatewayChannelConflictChecker/PaymentMethodTypeExtension POST_SUBMIT-ordering reasoning is unusually well commented and checks out against Symfony's actual form-submission algorithm.

$channel,
);

if (!$paymentMethod instanceof PaymentMethodInterface) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[HIGH] isOneyEnabled() only half-fixes channel scoping — the account-permission check still uses an arbitrary client

Lines 46-49 correctly resolve the channel-scoped payment method to check "does this channel have an enabled Oney config", but two lines below this check, the method's return delegates to $this->oneyChecker->isEnabled(), and OneyChecker is built on the fixed, non-scoped @payplug_sylius_payplug_plugin.api_client.oney singleton (src/Checker/OneyChecker.php:16-17) — the resolved $paymentMethod is never used there.

Failure scenario: two channels, each with its own Oney/PayPlug account, one with can_use_oney=true and the other false. isOneyEnabled() returns the same arbitrary answer for both channels — the Oney option is shown/hidden on the wrong channel. This contradicts the CHANGELOG's "the Oney availability check … now resolve[s] the gateway serving the current channel" claim: only the existence check was fixed, the permission check wasn't.

Same root cause (unscoped @payplug_sylius_payplug_plugin.api_client.oney singleton) also applies to src/Checker/OneyChecker.php, src/Provider/OneySimulation/OneySimulationDataProvider.php, src/Twig/OneyRulesExtension.php and src/Twig/ShowMeaExtension.php, left behind while their siblings (OneySupportedPaymentChoiceProvider, this method's own existence check) were fixed. Worth confirming whether this was intentionally deferred to PRE-3682 or simply missed.

controller itself so the two ends cannot drift apart. #}
{% if is_connected %}
{% set csrf_token_id = constant('PayPlug\\SyliusPayPlugPlugin\\Action\\Admin\\Auth\\UnifiedLogoutController::CSRF_TOKEN_ID_PREFIX') ~ payment_method.id %}
<a class="btn btn-outline-danger btn-sm mt-2"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MEDIUM] GET-based logout link has no extra hardening against the URL-carried-CSRF-token weaknesses

The token is Symfony's default session-stored CSRF token — non-single-use, never rotated after use — and it travels in the URL rather than a POST body, so it's exposed to referrer headers on any third-party resource loaded from this page, proxy/CDN access logs, and browser history (channels a POST body doesn't traverse). This mirrors Sylius's own ResendOrderConfirmationEmailAction pattern, so it's not a novel weakness, but this action's blast radius (destroys OAuth credentials + disables a payment method) is materially worse than "resend an email", and no extra mitigation was added: no rel="nofollow noreferrer" on the <a>, no Referrer-Policy anywhere in the app.

-        <a class="btn btn-outline-danger btn-sm mt-2"
+        <a class="btn btn-outline-danger btn-sm mt-2" rel="nofollow noreferrer"
             href="{{ path('payplug_sylius_admin_auth_logout', {


<div class="col-12 col-md-6 mt-5">
<div class="form-label">{{ 'payplug_sylius_payplug_plugin.ui.connected_account'|trans }}</div>
{% if account_email %}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[LOW] "Re-authenticate" placeholder is shown even for a gateway that was never connected at all

account_email and is_connected (line 10) are computed independently. For a brand-new, never-authorized config, account_email is falsy, so this branch renders "Re-authenticate to display the connected account" — implying a prior connection that lost its email, when the gateway was simply never connected. The logout button itself is correctly hidden in that case; only the text is misleading.

-    {% if account_email %}
+    {% if not is_connected %}
+        <div class="text-muted">{{ 'payplug_sylius_payplug_plugin.ui.not_connected'|trans }}</div>
+    {% elseif account_email %}
         <div class="fw-bold">{{ account_email }}</div>
     {% else %}

* disconnect. Pinned by GatewayConnectionRevokerTest; the proper fix is a `forget()` on UPC's
* TokenManager.
*/
private const TOKEN_CACHE_KEY_PREFIX = 'upc_oauth_token:';

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[LOW] TOKEN_CACHE_KEY_PREFIX duplicates a private constant in unified-plugin-core's TokenManager with no compiler-enforced link

Currently 'upc_oauth_token:' matches TokenManager::CACHE_KEY_PREFIX exactly, but nothing ties the two together across the module boundary. Already self-documented in this class's docblock as a workaround pending a real forget() on UPC's TokenManager — flagging so it stays tracked rather than silently drifting on a future UPC bump.


$paymentMethod = $this->paymentMethodRepository->findOneByGatewayName(ApplePayGatewayFactory::FACTORY_NAME);
$payment->setMethod($paymentMethod);
$payment->setMethod($this->resolveApplePayPaymentMethod($order));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[LOW] Duplicate resolution + unguarded null on the second call

provide() resolves resolveApplePayPaymentMethod($order) twice: once to gate on "Apple Pay is enabled" a bit earlier (line 52), and again right here to set the payment method, with no null-check on this second result. Harmless today (same order/channel, nothing changes in between) but it's two extra DB round-trips for the same answer, and the two call sites carry inconsistent guarantees.

-        if (!$this->resolveApplePayPaymentMethod($order) instanceof PaymentMethodInterface) {
+        $paymentMethod = $this->resolveApplePayPaymentMethod($order);
+        if (!$paymentMethod instanceof PaymentMethodInterface) {
             throw new LogicException('Apple Pay is not enabled');
         }
...
-        $payment->setMethod($this->resolveApplePayPaymentMethod($order));
+        $payment->setMethod($paymentMethod);

'oney_simulation_%s_%s_%s_%s',
$country,
$cart->getTotal(),
$this->oneySupportedPaymentChoiceProvider->getFeesFor(),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[LOW] Cache key's channel source differs from the cached value's channel source

The key includes $cart->getChannel()?->getCode() (line 35), but the value is partly driven by getFeesFor() on this line, which resolves its channel from the ambient ChannelContextInterface (current request), not from $cart. They coincide today because all current call sites use the current session's cart in the current channel, but it's a latent trap if getForCart() is ever called for a cart outside the matching HTTP request (console/queue/cron) — key and value could then silently disagree.

/** @var PaymentMethod|null $paymentMethod */
$paymentMethod = $this->paymentMethodRepository->findOneBy(['gatewayConfig' => $gateway]);
if (null === $paymentMethod || false === $paymentMethod->isEnabled()) {
if (!$channel instanceof ChannelInterface) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[NIT] Dead/misleading defensive check around getChannel()

ChannelContextInterface::getChannel(): ChannelInterface is non-nullable and throws ChannelNotFoundException rather than returning something that fails instanceof ChannelInterface — so this check is unreachable and gives a false impression the "no current channel" case is handled. The sibling fix in OneySupportedPaymentChoiceProvider::resolveOneyPaymentMethod() wraps the call in a real try { … } catch (ChannelNotFoundException) { return null; }; this method should either adopt the same pattern or drop the dead check.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants